-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
deps: upgrade puppeteer to v21.3.6 #15490
Conversation
@@ -190,7 +190,6 @@ async function buildBundle(entryPath, distPath, opts = {minify: true}) { | |||
// resolved eventually. | |||
plugins.partialLoaders.inlineFs({ | |||
verbose: Boolean(process.env.DEBUG), | |||
ignorePaths: [require.resolve('puppeteer-core/lib/esm/puppeteer/common/Page.js')], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Page.js
has moved but it looks like the comment that contained fs.readFileSync
is not longer there so we should be good to remove.
frameUrl: 'http://localhost:10200/dobetterweb/dbw_tester.html', | ||
}], | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive-by outdated bfcache failure
|
||
const browser = await puppeteer.launch({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Puppeteer will now disable the bfcache if we aren't using tab target which is currently only available through an environment variable PUPPETEER_INTERNAL_TAB_TARGET
. The env variable needs to be set before puppeteer is imported so we would need to do some annoying dynamic imports.
This is an alternative approach which just uses chrome-launcher
with our desired flags. This should be better for our smoke tests in general because it's closer to how Lighthouse launches Chrome.
related unit failure?
|
Yup, I'll investigate. |
@@ -164,7 +164,7 @@ describe('Individual modes API', function() { | |||
const run = await api.startTimespan(state.page); | |||
for (const iframe of page.frames()) { | |||
if (iframe.url().includes('/oopif-simple-page.html')) { | |||
iframe.click('button'); | |||
await iframe.click('button'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this await
the button click could happen after we end the timespan
This is a fix for the issue mentioned in #15490 (comment)
expect(networkRequests).toHaveLength(10); | ||
expect(networkRequests.filter(r => r.sessionTargetType === 'page')).toHaveLength(2); | ||
expect(networkRequests.filter(r => r.sessionTargetType === 'iframe')).toHaveLength(2); | ||
expect(networkRequests.filter(r => r.sessionTargetType === 'worker')).toHaveLength(6); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these checks are redundant with the snapshot check just below. The snapshot check provides better details on failure so we should just fall to that if a request is missing.
Issues that forced us to pin the version (#15458) should be fixed now.